-
Notifications
You must be signed in to change notification settings - Fork 1k
Reduce unsafe usages #14855
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Reduce unsafe usages #14855
Conversation
@SuppressWarnings("unused") | ||
public static class LoadClassAdvice { | ||
|
||
// Class loader stub is shaded back to the real class loader class. It is used to call protected |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried multiple approaches here (check the commits if interested). Firstly I tried creating a MethodHandles.Lookup
in the advice and passing it along. This allowed getting access to MethodHandles for findLoadedClass
and defineClass
. Unfortunately it didn't work on jdk8, apparently ClassLoader
is a restricted class there for which you can't create a Lookup
for. Didn't like that a special case was needed for jdk8. Next I tried writing the advice with asm. Since we need to call multiple methods it was a sizable chunk of asm so instead I tried what you see here. We use a modified stub ClassLoader
class that has findLoadedClass
and defineClass
as public so we could use them in the advice. During the build we shade the stub to java.lang.ClassLoader
. The bytecode works because this advice is inlined into subclasses of ClassLoader
that can call these protected methods. It isn't ideal either as it required some build script hacking to make the shading work.
// TODO by default, we use unsafe to define rest of the classes into boot loader | ||
// can be disabled with -Dnet.bytebuddy.safe=true | ||
// use -Dsun.misc.unsafe.memory.access=debug to check where unsafe is used | ||
if (ClassInjector.UsingUnsafe.isAvailable() && !classnameToBytes.isEmpty()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The following ClassInjector.UsingInstrumentation
works without unsafe but it needs to put the classes in to a jar and append that jar to boot class path. We could use that as a fallback, but the thing is that besides the virtual field classes that we defined above using lookup we have only a couple of helper classes that we need to add to boot loader. Could consider moving those classes to a bootstrap
module. With indy we would be loading those helpers into separate class loader and that would also get rid of the unsafe usage here.
This PR aims to reduce using unsafe to get us closer to not causing the unsafe warning getting printed with latest java.